Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(explore): SQL popover in datasource panel #19308

Merged
merged 5 commits into from
Apr 3, 2022
Merged

feat(explore): SQL popover in datasource panel #19308

merged 5 commits into from
Apr 3, 2022

Conversation

kgabryje
Copy link
Member

SUMMARY

Previously, we displayed metric's or calculateed column's SQL expression in a tooltip. This PR refactors that component to use a popover and present the SQL in ace editor - thanks to that the code is nicely formatted and easier to read. Also, the icon has been change from a question mark to a calculator.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
image
image

After:
image
image

TESTING INSTRUCTIONS

  1. Create a chart using a dataset with saved metrics and calculated columns
  2. Verify that the SQL popovers in dataset panel display correctly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

CC @kasiazjc

zhaoyongjie
zhaoyongjie approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codes LGTM! Thanks for bringing this Popover, and the SQL is more readable!

There are some no-blocking design suggestions:

  1. Could we add a scalable modal so that zoom in and out.

image

  1. The new icon for SQL expression doesn't look like SQL instead of a calculator.

image

cc: @kasiazjc

villebro
villebro approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely improvement! LGTM with a few non-blocking comments

superset-frontend/packages/superset-ui-chart-controls/package.json Outdated Show resolved Hide resolved
superset-frontend/packages/superset-ui-chart-controls/package.json Outdated
"@emotion/react": "^11.4.1",
"@superset-ui/core": "*",
"@types/enzyme": "^3.10.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bycatch nit: not yours, but could this be in devDependencies? Same for @types/react

@kgabryje
Copy link
Member Author

Codes LGTM! Thanks for bringing this Popover, and the SQL is more readable!

There are some no-blocking design suggestions:

  1. Could we add a scalable modal so that zoom in and out.
image

Thanks for the review! I like the idea of adding a resizer to the popover. However I'd suggest implementing it as a separate PR as there are a few edge cases to consider

@kgabryje kgabryje closed this Mar 28, 2022
@kgabryje kgabryje reopened this Mar 28, 2022
@kgabryje kgabryje added the lts-v1 label Apr 3, 2022
@kgabryje
Copy link
Member Author

kgabryje commented Apr 3, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

@kgabryje Ephemeral environment spinning up at http://35.87.97.214:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #19308 (14519aa) into master (90dbe8d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #19308   +/-   ##
=======================================
  Coverage   66.58%   66.58%           
=======================================
  Files        1677     1678    +1     
  Lines       64238    64242    +4     
  Branches     6538     6538           
=======================================
+ Hits        42773    42777    +4     
  Misses      19766    19766           
  Partials     1699     1699
Flag Coverage Δ
javascript 51.33% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/SqlLab/components/TemplateParamsEditor/index.tsx 75.00% <ø> (ø)
...-ui-chart-controls/src/components/ColumnOption.tsx 85.71% <100.00%> (ø)
...-ui-chart-controls/src/components/MetricOption.tsx 94.73% <100.00%> (ø)
...et-ui-chart-controls/src/components/SQLPopover.tsx 100.00% <100.00%> (ø)
...t-frontend/src/components/AsyncAceEditor/index.tsx 90.90% <100.00%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90dbe8d...14519aa. Read the comment docs.

@kgabryje kgabryje merged commit 60dcd65 into apache:master Apr 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

Ephemeral environment shutdown and build artifacts deleted.

@villebro villebro removed the lts-v1 label Apr 3, 2022
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@kgabryje @philipher29
feat(explore): SQL popover in datasource panel (apache#19308)
f9f7519 
* feat(explore): SQL popover in datasource panel

* Fix acequire not defined

* Rebase and fix tests

* Disable highlighting gutter

* Use ace-build acequire instead of brace
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@villebro villebro villebro approved these changes

@zhaoyongjie zhaoyongjie zhaoyongjie approved these changes

@geido geido Awaiting requested review from geido

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kgabryje @zhaoyongjie @villebro @mistercrunch